Skip to content

geoprobe: reject forged, replayed and spoofed geolocation input - #4271

Open
nikw9944 wants to merge 5 commits into
mainfrom
nikw9944/adhoc-413
Open

geoprobe: reject forged, replayed and spoofed geolocation input#4271
nikw9944 wants to merge 5 commits into
mainfrom
nikw9944/adhoc-413

Conversation

@nikw9944

@nikw9944 nikw9944 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Four defects in the RFC-16 geolocation chain accepted input that was never authenticated. Each fix has a test that fails without it.

1. Forged offsets were stored and served. geoprobe-target computed signatureValid and then never used it, so an unsigned UDP datagram to port 8923 reached the cache and location_offsets — a table the lake explorer aggregates with no filter on signature_valid, i.e. a public page. Invalid offsets are now dropped outright rather than written to a separate path: the writer has one table, an alternate one would be a schema change (out of scope), and abuse visibility comes from a Warn log instead.

While testing this I found the gate alone was bypassable. Go's ed25519.Verify does not screen the all-zero pubkey: it decodes to a valid order-4 point rather than being rejected, so with an all-zero signature the verification equation holds whenever the message hash lands on the right residue — about one message in four, which an attacker reaches by varying any field. A fully unsigned offset verified. The zero-key guard now covers every place in the measurement chain where a wire-supplied pubkey is used as the verification key: VerifyOffset, and ProbePacket.Verify / ReplyPacket.Verify in tools/twamp/pkg/signed. The fourth ed25519.Verify site, telemetry/state-ingest/pkg/server/auth.go, takes its key from device.MetricsPublisherPubKey after a successful lookup, so it is not attacker-supplied and is left alone.

Severity of the two packet-level sites, traced rather than assumed:

  • ProbePacket.Verify at the reflector is not exploitable end to end today. The allowlist check runs before verification, and the geolocation program already rejects a default target_pk for inbound targets (add_target.rs:84), so the zero key cannot reach authorizedKeys. The guard removes the reflector's dependence on an invariant enforced in another program.
  • ReplyPacket.Verify has real effect. LinuxSender.tryRecv pins reply.AuthorityPubkey to the expected probe key before verifying, but only on the verify=true path, which is Reply 1. Reply 0 is accepted unverified by design, and geoprobe-target-sender then calls Reply0.Verify() for audit logging — so a spoofed Reply 0 carrying a zero authority key was logged as reply0_sig=true.

2. RFC-16's replay mitigation was not implemented. MeasurementSlot was written and logged but never compared, and the only other freshness bound is receipt wall-clock, which a replay refreshes. The agent now rejects an inbound DZD offset unless its slot is within 15 min behind / 5 min ahead of the current ledger slot. The window comes from the caches on both ends: the DZD stamps offsets from a slot cached for SlotCacheTTL (5 min) and the agent compares against its own 5-min cache, so ±5 min of skew is legitimate before RPC and finalization lag; 15 min of lag leaves room for a degraded RPC without making the check meaningless. If no slot is available at all the offset is rejected — the agent cannot sign composite offsets in that state either.

Separately, offsetCache.Put used <= for best, and replacing best also resets its expiry clock, so a replayed offset held best forever. Now strictly <.

3. Unverified packets mutated per-sender reflector state. target_pk is public onchain, so ~2 spoofed packets per window could consume a paying sender's pair budget, repoint pairSourceIP, and clear its challenge nonce. The reflector verifies before touching state. Replying without verification is still allowed per RFC-16 — that path now runs off a throwaway state, so the nonce it issues authenticates nothing, and is capped at one reply per window per pubkey, since the reply is ~10x the probe size and an unlimited one would be a reflection amplifier.

4. Delinquent targets were measured forever. All-nil returns meant "scan skipped", which a completed-but-empty scan is indistinguishable from, so removing a user's last target or flipping them to Delinquent left the probe measuring until restart. discover now returns an explicit scanned bool.

Also: ICMP replies are matched on source address in addition to ID+seq.

No wire format or Borsh layout changed. RFC: rfc16-geolocation-verification.md — step 6 of the inbound flow is amended, since it stated the reflector does not verify probe signatures.

Testing Verification

  • Each fix was reverted individually with its test in place and confirmed failing, then restored and confirmed passing: forged offset reaching the cache and the ClickHouse buffer; a replayed offset (slot outside the window) reaching the cache over a live UDP listener, and an equal-RTT put refreshing best's clock; spoofed probes starving a legitimate sender's pair so its Reply 1 never comes back Challenged; an empty-but-completed scan not propagating on any of the three channels; an ICMP reply from an unprobed host being credited as the target's RTT; an unsigned probe and an unsigned reply verifying.
  • That revert check caught two weak tests. Because acceptance of the zero pair is message-dependent, TestVerifyOffset_ZeroPubkeyAndSignature and the first version of the probe test passed with their guard removed — their payloads hashed to a rejecting residue. All three zero-key tests now pin field values (offset slot 2, probe seq 1) that the unguarded code accepts, and each was re-confirmed failing without its guard.
  • go test ./controlplane/telemetry/... ./tools/twamp/... — all pass except tools/twamp/pkg/light, which fails identically before my changes in this sandbox (TestTWAMP_Sender_* cannot write to 10.255.255.255: operation not permitted). I did not touch that package.
  • golangci-lint run ./controlplane/telemetry/... ./tools/twamp/... clean.

@nikw9944
nikw9944 requested review from a team and a lite review from Copilot September 3, 2026 15:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The signed TWAMP reflector can still become a reflection amplifier when verifyInterval is set to 0 because the unverified-probe reply cap does not apply in that configuration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens the RFC-16 geolocation ingestion and measurement pipeline so that unauthenticated, replayed, and spoofed inputs cannot corrupt cached offsets, published ClickHouse data, or per-sender probe state.

Changes:

  • Drop unsigned or invalid signature-chain LocationOffset datagrams before caching or writing to ClickHouse, and reject the all-zero Ed25519 authority pubkey case.
  • Enforce replay mitigation for inbound offsets by validating MeasurementSlot against the current ledger slot, and prevent equal-RTT replays from refreshing the “best” cache entry.
  • Prevent spoofed signed-TWAMP probes from mutating per-sender pair state, and tighten ICMP RTT attribution by matching reply source address in addition to ID and sequence.
File summaries
File Description
tools/twamp/pkg/signed/reflector_test.go Adds regression coverage for spoofed probes not disturbing a legitimate sender’s pair.
tools/twamp/pkg/signed/reflector_linux.go Verifies probe signatures before touching per-sender pair state; adds unverified-reply handling.
rfcs/rfc16-geolocation-verification.md Updates inbound flow documentation to reflect reflector verification and state isolation.
controlplane/telemetry/internal/geoprobe/target_discovery.go Distinguishes “scan skipped” from “scan ran but matched nothing” via an explicit scanned return value.
controlplane/telemetry/internal/geoprobe/target_discovery_test.go Updates call sites and adds an empty-scan propagation regression test.
controlplane/telemetry/internal/geoprobe/signer.go Rejects zero authority pubkey prior to Ed25519 verification.
controlplane/telemetry/internal/geoprobe/signer_test.go Adds coverage for the zero (pubkey, signature) acceptance edge case.
controlplane/telemetry/internal/geoprobe/metrics.go Adds rejection reason labels for slot freshness enforcement.
controlplane/telemetry/internal/geoprobe/icmp_pinger.go Matches ICMP replies on source IP to prevent spoofed RTT attribution.
controlplane/telemetry/internal/geoprobe/icmp_pinger_test.go Adds tests to ensure mismatched-source replies are ignored.
controlplane/telemetry/internal/geoprobe/icmp_conn.go Extends recv path to return sender IPv4 address from Recvmsg.
controlplane/telemetry/internal/geoprobe/icmp_conn_test.go Updates tests for new recvEcho return values and validates source address.
controlplane/telemetry/internal/geoprobe/clickhouse.go Adds BufferedRows() to support assertions for “dropped vs recorded” behavior in tests.
controlplane/telemetry/cmd/geoprobe-target/main.go Drops invalid signature-chain offsets before cache and ClickHouse write.
controlplane/telemetry/cmd/geoprobe-target/main_test.go New tests ensuring forged offsets do not reach cache or ClickHouse buffer.
controlplane/telemetry/cmd/geoprobe-agent/main.go Enforces MeasurementSlot freshness, adjusts best-cache replacement semantics, and threads slot lookup into listener.
controlplane/telemetry/cmd/geoprobe-agent/main_test.go Adds unit and listener-path tests for slot-window rejection and equal-RTT non-refresh.
CHANGELOG.md Documents the geolocation hardening changes and their security rationale.
Review details
  • Files reviewed: 18/18 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tools/twamp/pkg/signed/reflector_linux.go
Comment thread controlplane/telemetry/internal/geoprobe/target_discovery.go

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 76be6cf. Configure here.

Comment thread controlplane/telemetry/cmd/geoprobe-agent/main.go
Comment thread controlplane/telemetry/internal/geoprobe/signer_test.go Outdated
@nikw9944
nikw9944 requested a review from ben-dz September 5, 2026 14:36
Four defects in the RFC-16 measurement chain let unauthenticated input
through:

- geoprobe-target computed signatureValid and never gated on it, so a
  bare UDP datagram landed in the cache and in location_offsets, which
  the lake explorer publishes unfiltered. Verification also accepted the
  all-zero (pubkey, signature) pair, which ed25519.Verify treats as
  valid, so the gate alone would have been bypassable.
- MeasurementSlot, RFC-16's named replay mitigation, was never compared
  against anything. The agent now rejects offsets outside a bounded slot
  window, and an equal-RTT offset no longer resets the cache entry's
  expiry clock.
- The signed TWAMP reflector wrote per-sender pair state from packets it
  never verified. It verifies first now; unverified probes still get a
  reply, off throwaway state and rate-limited.
- Target discovery used all-nil returns as the "scan skipped" sentinel,
  which a completed empty scan is indistinguishable from, so a
  deregistered or delinquent user was probed until restart.

Also match ICMP echo replies on source address, not just ID and seq.
ProbePacket.Verify and ReplyPacket.Verify both key off a pubkey copied
straight off the wire, and neither screened the all-zero key. Go's
ed25519.Verify does not reject it: the zero key decodes to a valid
order-4 point, so with an all-zero signature the verification equation
holds for roughly one message in four — an attacker just varies a
sequence number until it lands.

Both now use the same guard already added to VerifyOffset, which is the
third and last place in the measurement chain where a wire-supplied
pubkey is the verification key.

Also correct that guard's comment, and pin the field values in all three
zero-key tests. Two of them passed with the guard removed because their
payloads happened to hash to a rejecting residue, which proved nothing.
Review follow-ups on the RFC-16 hardening:

- getCurrentSlot serves its cache forever when ledger RPC fails, so the
  new replay window froze around whatever slot was last fetched: replays
  near it stayed acceptable for the length of the outage, and fresh
  offsets eventually fell outside the lead tolerance. Offset ingestion
  now refuses a reference older than two refresh periods. Composite
  offsets keep the old behavior, since stamping a slightly stale slot
  beats emitting nothing during a blip.
- The unverified-reply cap was keyed off verifyInterval, which is 0 when
  pair rate limiting is disabled — leaving that path uncapped, which is
  the amplifier the cap exists to prevent. It now has a 1s floor.
- discover's doc comment claimed scanned distinguishes a skipped scan
  from a completed one, without saying it is only meaningful when err is
  nil. Document that rather than return true from a scan that failed.
- Assert the specific error in the zero-pubkey offset test.
@nikw9944

nikw9944 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Addressed all four review points in 4206585, and rebased onto main (v0.39.0) to replace the Update Branch merge commit with linear history — the rebased tree is byte-identical to that merge commit, and the CHANGELOG entry moved into the new Unreleased section since the release cut turned the old one into v0.39.0.

The substantive one was Bugbot's stale-slot finding: getCurrentSlot falls back to its cache indefinitely on RPC failure, so the replay window I added froze around the last fetched slot — replays near it stayed acceptable for the length of an outage, and fresh offsets would eventually be rejected as too far ahead. Ingestion now rejects a slot reference older than two refresh periods (10m); composite offsets keep the old behavior so the probe still emits during a blip.

Copilot's amplifier point was also correct: the unverified-reply cap keyed off verifyInterval, which is 0 when pair rate limiting is disabled, leaving the path it was meant to protect uncapped. It now has a 1s floor. Both fixes have tests that fail against the previous code.

Chose differently on one: discover still returns scanned: false on an RPC error. Returning true would claim a scan ran when it did not, and discoverAndSend checks err first, so the comment now documents that scanned is meaningful only when err is nil.

go test ./controlplane/telemetry/... ./tools/twamp/... passes except tools/twamp/pkg/light, which fails in my sandbox for lack of raw-socket permissions and is untouched by this branch (git diff origin/main -- tools/twamp/pkg/light is empty).

Only CHANGELOG.md conflicted: main added entries to the same Unreleased
Changes section. Both sides' blocks stay there — this PR is unmerged, so
its entry does not belong in a released version.
Only CHANGELOG.md conflicted: main added a CLI entry to the same
Unreleased Changes section. Both sides' blocks stay there — this PR is
unmerged, so its entry does not belong in a released version.
@nikw9944

Copy link
Copy Markdown
Contributor Author

Branch updated for main again (c5815ae5) — main had moved 8 commits and CHANGELOG.md was the only conflict, as before. Main added a CLI entry to the same UnreleasedChanges section; both blocks stay there, since this PR is unmerged and its entry does not belong in a released version. Verified mechanically: my Geolocation block is byte-identical to the previous head, the merged section is exactly that block plus main's verbatim, no entry from either side was dropped, and every released section matches main.

No code changed. For all 19 non-CHANGELOG files this PR touches, git diff 2d863301 HEAD -- <file> is empty.

No new review feedback since the last update — the four inline threads already carry replies, and nothing needing action arrived after them.

Local checks on the merge result: go test -race ./tools/twamp/pkg/signed/ and the four geolocation packages pass; golangci-lint run -c ./.golangci.yaml ./tools/twamp/pkg/signed/... ./controlplane/telemetry/... reports 0 issues. e2e still needs a Docker host and has not been run here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants